Could you please undo the indent changes please

With the addition of the try block the indentations were necessary, excluding them the commit will result in lint errors.

I missed that.. why is the extra try block needed? Wouldn't it be possible to add a .finally() to the p.promise instead?

Yes, I can add the finally to the promise, though the initial start event is placed in the first try block

Can you resolve the conflicts with main?

@mcollina conflicts have been resolved 👍🏼

Since your work started, we have migrated the tests to node:test. Could you converT?

My proposed suggestion would migrate the test from tap to node:test ;)

@Uzlopak committed the changes, thanks for that!

Interesting. The unit fests found a valid bug.

Thanks for the great work and keeping up with us!

@tsctx ptal

I won't block but I am a huge -1 on this. "Node-ifying" fetch will turn it into one of the other half-dozen implementations node has that aren't really maintainable or maintained.

This PR changes around 80 lines of code in the actual algorithm. It doesn't seem this is making fetch() unmaintainable.

@jasnell can you take a quick look too?

It absolutely will make maintaining fetch harder.

How? For the most part this is just an indentation level change.

It's not the code itself, it's how intrusive it is. We are taking a web spec and shoving node-specific things into it, things that are not entirely compatible with the spec (I assume that's why you said "for the most part"). There are steps that are rewritten and things added to fetch itself. Every, every single time we have even slightly deviated from the spec it always comes back to bite us in the ass later on, even the smallest change to the spec makes maintaining it harder. I can find specific examples but I hope you'll trust me on that...

@crysmags I think it could be implemented with fewer changes. In the way... tsctx@56c36f7

Please update the document.

@tsctx Which document are you referring to?

Documentation of how to use the added debuglog's.

Yeah, I also think that minimal invasiveness like the solution of @tsctx is more preferable.

That was also my thought with #2857

But at one point you added code with runStores() and I did not investigate why that is now the preferred way to do it.

The runStores method is required for AsyncLocalStorage to be able to alter its state to a value produced from Tracing Channel. APMs use this to transform Tracing Channel event data into our Span instances. The version proposed by @tsctx would not be usable by APM vendors due to the lack of runStores, and also has incorrect end event timing as it needs to be the sync end of the function call, not the start of the next tick.

Should we do a call to discuss this?

I'm happy to get on a call with folks about it, if that is needed.

Here is a doodle: https://doodle.com/meeting/participate/id/dwRDjnJb.

Who is also invited? Am i also invited?

Yes

Is it intentional that the selected dates in the Doodle don't start until July? Seems like quite awhile to wait to make a decision on this. 🤔

I'm traveling the next 2 weeks.

Fair enough. We can find time after your travels then. Safe travels! 👋🏻

What's happening with this? We still planning a meeting of some sort? @mcollina

Any updates? I'm looking forward to this feature for network inspection fetch support.

@KhafraDev can we do a meeting next week to figure out what can be done to unblock this? There are several companies asking me to make this happen.

I don't have time - the semester is ramping up for me and finals are happening in a little over a month. There are still issues I have with this PR:

• This has gone unresolved https://github.com/nodejs/undici/pull/2701/files#r1636015825
• Adding diagnostics channels to Fetch #2701 (comment)

The further from the spec we deviate, and the more node-specific features we implement, the harder it becomes to maintain. It also becomes harder for other runtimes to maintain compatibility. Fortunately, undici (outside of the web specs) provides a fantastic api that fits these requirements, or at least is more open to changes that would satisfy them.

This has gone unresolved https://github.com/nodejs/undici/pull/2701/files#r1636015825

This is an easy change that we can make.

#2701 (comment)

Nothing really changes with the API or the functionality, the PR simply adds observability to it, which is a cross-cutting concern. I would also argue that observability should be baked into at the very least any library doing IO, which undici does. What would be a way forward if the diagnostics capability provided by the runtime cannot be considered?

I would also argue that observability should be baked into at the very least any library doing IO, which undici does

https://github.com/nodejs/undici/blob/main/docs/docs/api/DiagnosticsChannel.md

Nothing really changes with the API or the functionality

Emitting a warning when adding more than 10 listeners to an EventTarget doesn't change the API or the functionality, but it still causes issues like nodejs/node#54758. There is more to consider when making changes to specs, especially living standards.

What would be a way forward if the diagnostics capability provided by the runtime cannot be considered?

fetch is a browser spec, the better option is to use the other APIs undici provides.

• This has gone unresolved https://github.com/nodejs/undici/pull/2701/files#r1636015825

I've added a comment which covers that. Seems like an easy enough change to make, though I also don't agree on the significance--Node.js core is wildly inconsistent about promise timing, being a tick off is going to make zero difference and microtask timing is really not something that should be depended on in the first place. V8 itself has changed its microtask timing a few times as the spec has evolved, so depending on that timing is just not a good idea.

• Adding diagnostics channels to Fetch #2701 (comment)

Do you have a concrete and actionable objection to provide for that? What about the current code is too intrusive? What can we do to make it less so? Yes, this is doing node-specific things in a web spec implementation, but this is a web spec implementation for use in Node.js so I don't see how that's a concern. What about the changes are logically inconsistent with the spec?

Yes, adding this increases code complexity a bit, but this capability is needed by all observability providers which are in use by almost all Node.js applications. The majority of users of undici need this capability.

fetch is a browser spec, the better option is to use the other APIs undici provides.

That provides a view of the internals, not of the interface actually being used by the end user. It's not a useful level to be capturing for observability that is useful and actionable to the user.

This PR has been stuck in limbo for a long time. Can we have the @nodejs/undici maintainers please make a vote to decide how we proceed with this?